Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[FB] [PI-3585] Change default loading unit from "1" to "unknown" (correct branch) #3709

Merged
merged 9 commits into from
Jun 1, 2020
Merged

[FB] [PI-3585] Change default loading unit from "1" to "unknown" (correct branch) #3709

merged 9 commits into from
Jun 1, 2020

Conversation

stephenworsley
Copy link
Contributor

The same as #3705 except it is targeting a branch specifically for changes to default units. Because of this, the test is changed to no longer require the loading of ancillary variables.

@pp-mo pp-mo mentioned this pull request May 29, 2020
Merged
pp-mo
pp-mo reviewed May 29, 2020
View reviewed changes
lib/iris/fileformats/_pyke_rules/fc_rules_cf.krb Outdated
@@ -1651,9 +1651,9 @@ fc_extras

################################################################################
def get_attr_units(cf_var, attributes):
attr_units = getattr(cf_var, CF_ATTR_UNITS, cf_units._UNIT_DIMENSIONLESS)
attr_units = getattr(cf_var, CF_ATTR_UNITS, cf_units._UNKNOWN_UNIT_STRING)
Copy link
Member

@pp-mo pp-mo May 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see no need here to reference private bits of cf_units
-- though I admit that is what the existing code is doing !
We should set up our own global for this, with something like UNKNOWN_UNIT_STRING = str(cf_units(None))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to change all the other references to private bits of cf_units to something similar? i.e. cf_units._NO_UNIT_STRING.

pp-mo
pp-mo reviewed May 29, 2020
View reviewed changes
.../whatsnew/contributions_3.0.0/incompatiblechange_2020-May-15_change_default_unit_loading.txt Outdated
@@ -0,0 +1 @@
* NetCDF files containing variables without units will be given a unit of "unknown" rather than "1"
Copy link
Member

@pp-mo pp-mo May 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs fleshing out to make it clearer how it may affect the user.
Something like ...
"When loading data from netcdf-CF files, where a variable has no "units" property, the corresponding Iris object will have ".units='unknown'". Prior to Iris 3.0, these cases defaulted to "units=1".
This applies to cubes, coordinates, and ancillary data."

pp-mo
pp-mo reviewed Jun 1, 2020
View reviewed changes
Copy link
Member

@pp-mo pp-mo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One really tiny additional suggestion, otherwise I think this is all good.

lib/iris/fileformats/_pyke_rules/fc_rules_cf.krb Outdated
@@ -1195,6 +1195,8 @@ fc_extras
UD_UNITS_LON = ['degrees_east', 'degree_east', 'degree_e', 'degrees_e',
'degreee', 'degreese', 'degrees', 'degrees east',
'degree east', 'degree e', 'degrees e']
UNKNOWN_UNIT_STRING = str(cf_units.as_unit(None))
Copy link
Member

@pp-mo pp-mo Jun 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A tiny point, but I do think it would be better to be definite here, and use 'unknown' instead of None.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional:
I think, from the code here, we don't actually need these to match some canonical form, or even be Unit instances, as they are only used as defaults and assigned values.
In that case, you can simply have strings, with your own choice of form, and don't need to wrap them in as_unit at all.

Also, looking again at cf_units, another simple alternative is to use the "unit symbol"s,
which is perhaps clearer and more unambiguous.
With those, you can just have "?" for unknown, and "-" for no-unit,
which at least avoids any uppercase / lowercase uncertainties.

@pp-mo pp-mo merged commit 3171b95 into SciTools:default_units Jun 1, 2020
@pp-mo
Copy link
Member

pp-mo commented Jun 1, 2020

Thanks @stephenworsley !
I still wasn't totally sure of my logic there, but if the tests pass I think that shows this is OK

@pp-mo pp-mo added this to the v3.0.0 milestone Jun 3, 2020
stephenworsley added a commit to stephenworsley/iris that referenced this pull request Jun 8, 2020
@stephenworsley
Merge branch 'default_units' of https://github.com/SciTools/iris into…
4f267ad 
… default_units_patch

* 'default_units' of https://github.com/SciTools/iris:
  Unify saving behaviour of "unknown" and "no_unit" (SciTools#3711)
  Change default loading unit from "1" to "unknown" (correct branch) (SciTools#3709)
  Change default units to "unknown" for all DimensionalMetadata (SciTools#3713)
  Update docs CubeList.extract method (SciTools#3694)
  Correct and improve dev-guide section on fixing graphics-tests. (SciTools#3683)
  New image hashes for mpl 3x2 (SciTools#3682)
  Switched use of datetime.weekday() to datetime.dayofwk. (SciTools#3687)
  Remove TestGribMessage (SciTools#3672)
  Removed iris.tests.integration.test_grib_load and related CML files. (SciTools#3670)
  Removed grib-specific test to iris-grib. (SciTools#3671)
  Fixed asv project name to 'scitools-iris'. (SciTools#3660)
  Remove cube iter (SciTools#3656)
  Remove test_grib_save.py (SciTools#3669)
  Remove test_grib2 integration tests (SciTools#3664)
  Remove uri callback test which is moved to iris-grib (SciTools#3665)
  2v4 mergeback picks (SciTools#3668)
  Remove test_grib_save_rules.py which has been moved to iris-grib (SciTools#3666)
  Removed ununused skipIf. (SciTools#3632)
  Remove grib-specific test. (SciTools#3663)
  Remove obsolete test. (SciTools#3662)
abooton added a commit to abooton/iris that referenced this pull request Jun 10, 2020
@abooton
Updating default_units to base a07800c
6c3832e 
Squashed commit of the following:

commit 912f500
Author: stephenworsley <[email protected]>
Date:   Mon Jun 1 17:52:42 2020 +0100

    Unify saving behaviour of "unknown" and "no_unit" (SciTools#3711)

commit 3171b95
Author: stephenworsley <[email protected]>
Date:   Mon Jun 1 16:28:45 2020 +0100

    Change default loading unit from "1" to "unknown" (correct branch) (SciTools#3709)

commit 3fdccac
Author: stephenworsley <[email protected]>
Date:   Mon Jun 1 12:27:23 2020 +0100

    Change default units to "unknown" for all DimensionalMetadata (SciTools#3713)
abooton added a commit to abooton/iris that referenced this pull request Jun 10, 2020
@abooton
Updating default_units to base a07800c
6a924b7 
Squashed commit of the following:

commit 912f500
Author: stephenworsley <[email protected]>
Date:   Mon Jun 1 17:52:42 2020 +0100

    Unify saving behaviour of "unknown" and "no_unit" (SciTools#3711)

commit 3171b95
Author: stephenworsley <[email protected]>
Date:   Mon Jun 1 16:28:45 2020 +0100

    Change default loading unit from "1" to "unknown" (correct branch) (SciTools#3709)

commit 3fdccac
Author: stephenworsley <[email protected]>
Date:   Mon Jun 1 12:27:23 2020 +0100

    Change default units to "unknown" for all DimensionalMetadata (SciTools#3713)
@abooton abooton changed the title Change default loading unit from "1" to "unknown" (correct branch) [FB] [PI-3585] Change default loading unit from "1" to "unknown" (correct branch) Jun 10, 2020
abooton added a commit to abooton/iris that referenced this pull request Aug 19, 2020
@abooton
Merge remote-tracking branch 'upstream/default_units' into default_un…
c3a3163 
…its_merge

* upstream/default_units:
  fix test (SciTools#3732)
  Unify saving behaviour of "unknown" and "no_unit" (SciTools#3711)
  Change default loading unit from "1" to "unknown" (correct branch) (SciTools#3709)
  Change default units to "unknown" for all DimensionalMetadata (SciTools#3713)
trexfeathers pushed a commit that referenced this pull request Aug 19, 2020
@abooton @stephenworsley
Update default_units feature branch base1873ce14 (#3792)
e99f960 
* Change default units to "unknown" for all DimensionalMetadata (#3713)

* Change default loading unit from "1" to "unknown" (correct branch) (#3709)

* Unify saving behaviour of "unknown" and "no_unit" (#3711)

* fix test (#3732)

Co-authored-by: stephenworsley <[email protected]>
stephenworsley added a commit that referenced this pull request Aug 20, 2020
@abooton @stephenworsley
Merge "default_units" feature branch (#3795)
42a1964 
* Change default units to "unknown" for all DimensionalMetadata (#3713)

* Change default loading unit from "1" to "unknown" (correct branch) (#3709)

* Unify saving behaviour of "unknown" and "no_unit" (#3711)

* fix test (#3732)

Co-authored-by: stephenworsley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pp-mo pp-mo pp-mo left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@stephenworsley @pp-mo